MDEV-40063 Corruption due to race in SET GLOBAL innodb_log_archive=ON#5253
Conversation
|
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the checkpoint writing logic in InnoDB (buf0flu.cc and log0log.cc) to block concurrent set_archive() calls by setting resize_log = log earlier and handling its cleanup. The review feedback suggests refactoring the control flow to avoid using a goto statement to jump into an if block, which would improve code readability and maintainability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!archive) | ||
| { | ||
| archived_lsn= end_lsn; | ||
| checkpoint_completed: | ||
| if (resize_log.m_file == log.m_file) | ||
| { | ||
| /* We may have assigned resize_log= log to keep set_archived() out. */ | ||
| #ifdef HAVE_PMEM | ||
| ut_ad(!is_mmap()); | ||
| #endif | ||
| resize_log.m_file= OS_FILE_CLOSED; | ||
| } | ||
| } |
There was a problem hiding this comment.
Jumping into an if block using goto is a highly discouraged control flow pattern in C++ that hurts readability and maintainability. We can completely avoid the goto statement and the checkpoint_completed label by restructuring the code. Since resize_log.m_file != log.m_file is guaranteed when archive_header_was_reset is true, we can safely place the resize_log.m_file cleanup block directly after the if-else chain. This simplifies the logic and makes it much cleaner.
if (!archive)
{
archived_lsn= end_lsn;
}| else | ||
| goto checkpoint_completed; |
There was a problem hiding this comment.
By placing the cleanup block here after the if-else chain, we can completely eliminate the goto checkpoint_completed; statement and the corresponding label inside the if (!archive) block. This makes the control flow linear and much easier to follow.
if (resize_log.m_file == log.m_file)
{
/* We may have assigned resize_log= log to keep set_archived() out. */
#ifdef HAVE_PMEM
ut_ad(!is_mmap());
#endif
resize_log.m_file= OS_FILE_CLOSED;
}
There was a race condition between log_t::write_checkpoint() and the execution of SET GLOBAL innodb_log_archive=ON (enabling log archiving). We had wrongly allowed the concurrent execution of log_t::set_archive() and log_t::write_checkpoint(). The result was that log_sys.next_checkpoint_no was corrupted. This could have broken crash recovery. log_t::write_checkpoint(): When we are releasing log_sys.latch while durably writing the checkpoint header block, assign log_sys.resize_log to log_sys.log to inform other threads that a checkpoint is in progress. Previously, we only did this when innodb_log_archive=ON holds. log_t::resize_start(): Relax a debug assertion for the logic change. Tested by: Matthias Leich
3 participants
There was a race condition between
log_t::write_checkpoint()and the execution ofSET GLOBAL innodb_log_archive=ON(enabling log archiving). We had wrongly allowed the concurrent execution oflog_t::set_archive()andlog_t::write_checkpoint(). The result was thatlog_sys.next_checkpoint_nowas corrupted. This could have broken crash recovery.log_t::write_checkpoint(): When we are releasinglog_sys.latchwhile durably writing the checkpoint header block, assignlog_sys.resize_logtolog_sys.logto inform other threads that a checkpoint is in progress. Previously, we only did this wheninnodb_log_archive=ONholds.log_t::resize_start(): Relax a debug assertion for the logic change.This addresses this problem found in #4817 and originally introduced in #4405.